-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: APP-255 add supported file types link #2466
Conversation
✅ Deploy Preview for regen-website ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@erikalogie see testing instructions |
</Trans> | ||
<Link href="#"> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it'd be better to use padding
instead of
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? Using a white-space means it will adjust based on the font size so we would need to provide two padding values for mobile/desktop to get the same result.
I thought this made sense since the two elements are displayed inline, just like one single sentence but maybe there are other reasons I'm not aware of?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As
adjusts with font size we can end up losing control over the space between the elements if for example the browser settings override our font sizes. Also, using CSS for spacing keeps content and style separate, which is a good practice. I think using margin or padding is a more reliable way to control spacing between inline elements and it's easier to update and maintain if the design changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
</Trans> | ||
<Link href="#"> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As
adjusts with font size we can end up losing control over the space between the elements if for example the browser settings override our font sizes. Also, using CSS for spacing keeps content and style separate, which is a good practice. I think using margin or padding is a more reliable way to control spacing between inline elements and it's easier to update and maintain if the design changes.
LGTM |
05d58d0
to
bc2f7b4
Compare
Description
https://regennetwork.atlassian.net/browse/APP-255
Also fixes some small translation issue
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
How to test
From https://deploy-preview-2466--regen-marketplace.netlify.app/, log in and create a post. From the post form, check that clicking "View all supported file types»" opens the right link in a new tab.
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...